-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make termination condition config declarative #4984
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4984 +/- ##
===========================================
- Coverage 72.42% 56.05% -16.37%
===========================================
Files 112 67 -45
Lines 6494 5755 -739
===========================================
- Hits 4703 3226 -1477
- Misses 1791 2529 +738
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
component_config_schema = AndTerminationConditionConfig | ||
component_type = "termination" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we include component override for cleaner path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
@ekzhu , for Or/AndTerm, are you fine with making it a public TerminationCondition and exporting this to give it a clean path? Anyway, these are used in developer code (or_term = stop_term and max_term
) so might be a good idea?
Something like from ._termination import TerminatedException, TerminationCondition, OrTerminationCondition, AndTerminationCondition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just to make sure, we will use the override for a cleaner provider path right?
|
||
|
||
class TerminationCondition(ABC): | ||
class TerminationCondition(ABC, Component[Any]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class TerminationCondition(ABC, Component[Any]): | |
class TerminationCondition(ABC, Component[BaseModel]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was gonna ask about this ... it leads to a flurry of pyright/mypy errors .. was not sure how to deal :) . Given that each base class have their own ConfigModels ..
Also, ComponentLoader does not want an abstract class (TerminationCondition) and it will be too disruptive to make a concrete BaseTerminationCondition and then refactor the rest of the code to do this.
conditions = [
ComponentLoader.load_component(
condition_model, TerminationCondition)
for condition_model in config.conditions
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the pattern should be that the base interface is a ComponentLoader
but not a Component
since itself is not loadable after all!
But this does make it difficult to generically dump a component...
Should be written as (after making the interface a ComponentLoader):
TerminationCondition.load_component(...)
Unsure if that fixes your issue though
Why are these changes needed?
Step 1 in making components in agentchat natively declarative.
Results
Related issue number
Closes #4983
Checks